provider/aws: Add support for Network Loadbalancers #1806
Conversation
|
Not sure what happened in #1629 - my remote branch got deleted so i pushed a new version and it closed the PR |
I'm pretty confident it's the same, based on patch I was able to pull from my local branch when reviewing the old PR: https://gist.github.com/radeksimko/f0e953ba5bae73c05732648d5bafc032 So feel free to make the changes. |
radeksimko
added
enhancement
|
Hi @radeksimko Ok, I made the docs changes. The issue with this line: We set a default value for Does this make more sense? Do you think there is a better way to handle this? P. |
aws/resource_aws_lb_listener.go
Outdated
| port := v.(int) | ||
| if port < 1 || port > 65536 { | ||
| errors = append(errors, fmt.Errorf("%q must be a valid port number (1-65536)", k)) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| func validateAwsAlbListenerProtocol(v interface{}, k string) (ws []string, errors []error) { | ||
| func validateAwsLbListenerProtocol(v interface{}, k string) (ws []string, errors []error) { | ||
| value := strings.ToLower(v.(string)) | ||
| if value == "http" || value == "https" { |
There was a problem hiding this comment.
Network Load Balancers only support TCP listeners, so I think line 265 needs to be updated to allow for a value of "tcp".
https://github.com/aws/aws-sdk-go/blob/master/service/elbv2/api.go#L3294-L3296
There was a problem hiding this comment.
@jwinter you are indeed correct - just pushed this change!
Fixes: hashicorp#1618 In terraform, we had the idea of an alb. In AWS this doesn't exist. ALBs are actually Load balancers of type `application` Therefore, the first part of this PR adds a new parameter to ALBs called `load_balancer_type`. We default this to `application` to follow the same idea as the current behaviour The next part of the PR will then change the idea of an alb -> lb In order to preserve backwards compatibility, we have added another resource name to the same schema type. This means we effectively have an alias of aws_alb and aws_lb. This includes updating *all* of the tests to make sure and remove the idea of ALB and rename to LB and then we will add a check to make sure we can still check that an ALB can be created in the old resource
radeksimko
removed
the
waiting-response
There was a problem hiding this comment.
We set a default value for idle_timeout and the way that Create calls Update func, we don't want to set that value if the type of loadbalancer is network
I see, I think there's a better way, but I don't want to block this PR on that - I'll raise a separate one. 👍
|
@radeksimko I agree - let's certainly chat about this and see if we can solve this in another fashion as a followup |
|
Re: " If state is going to need to be modified, will there be tooling or auto-background-migration at a given Terraform version, or will we need to do manual |
|
@handlerbot I think we first need to implement mechanism for deprecating resources. Then we're actually going to mark those old names as deprecated and they'll be reported as such when used in configs. This is work to be done in core schema package. Until that's done, aliases will remain in place as we want to avoid surprises. There is no migration tool/mechanism available at this point for any deprecations, although we did discuss the idea of having one internally within the team. If we don't have one at the time of real deprecation then yes, it will require I don't have any timelines to share around any of the above, but I'm pretty sure that from introducing resource deprecation (in the schema package) it would take at least a couple of months, or rather half a year until the aliases are removed. |
| port := v.(int) | ||
| if port < 1 || port > 65536 { | ||
| errors = append(errors, fmt.Errorf("%q must be a valid port number (1-65536)", k)) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| func validateAwsAlbTargetGroupProtocol(v interface{}, k string) (ws []string, errors []error) { | ||
| func validateAwsLbTargetGroupProtocol(v interface{}, k string) (ws []string, errors []error) { | ||
| protocol := strings.ToLower(v.(string)) | ||
| if protocol == "http" || protocol == "https" { |
There was a problem hiding this comment.
this needs to allow "tcp", just like your change to validateAwsLbListenerProtocol. "tcp" is a valid network load balancer target group protocol.
Update aws_lb docs after regression introduced in #1806 around logs
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
ghost
locked and limited conversation to collaborators
Fixes: #1618
In terraform, we had the idea of an alb. In AWS this doesn't exist. ALBs
are actually Load balancers of type
applicationTherefore, the first part of this PR adds a new parameter to ALBs called
load_balancer_type. We default this toapplicationto follow thesame idea as the current behaviour
The next part of the PR will then change the idea of an alb -> lb
In order to preserve backwards compatibility, we have added another
resource name to the same schema type. This means we effectively have an
alias of aws_alb and aws_lb. This includes updating all of the tests
to make sure and remove the idea of ALB and rename to LB and then we
will add a check to make sure we can still check that an ALB can be
created in the old resource